Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change dof_residual definition to not depend on dispersion parameter #560

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Change dof_residual definition to not depend on dispersion parameter #560

merged 1 commit into from
Aug 27, 2024

Conversation

andreasnoack
Copy link
Member

Fixes #509

There was actually already a test of a model affected by this but the reference value was wrong. I checked the value in R.

@nalimilan how do you feel about dof after all these years? After preparing this, I think we might be better of from deprecating dof (which would have to happen in StatsAPI). I tend to think that it brings more confusion than convenience.

@codecov Codecov
Copy link

codecov bot commented May 8, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.85%. Comparing base (e2f6c98) to head (5c26648).
Report is 3 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
- Coverage   89.86%   89.85%   -0.01%     
==========================================
  Files           8        8              
  Lines        1125     1124       -1     
==========================================
- Hits         1011     1010       -1     
  Misses        114      114

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andreasnoack andreasnoack mentioned this pull request Aug 27, 2024
Closed
nalimilan
nalimilan approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. Sounds reasonable to me to include this as part of 2.0 given that it's breaking.

Regarding dof, you mean we should deprecate it because people confuse it with dof_residual?

@andreasnoack
Copy link
Member Author

Regarding dof, you mean we should deprecate it because people confuse it with dof_residual?

Yeah. The confusion seems to show up repeatedly and I think that dof_residual is all you really need.

@andreasnoack andreasnoack merged commit 5c33d9e into master Aug 27, 2024
12 checks passed
@andreasnoack andreasnoack deleted the an/dof_residual branch August 27, 2024 09:59
@nalimilan
Copy link
Member

I see. But it can be useful in some cases, notably it's used in the definition of aic, bic and adjr2. In R you have to access private fields of model objects to get that information, which is bad.

Maybe we could put a warning in the docstring? Or we could have a single dof function with an argument to choose whether you want the consumed or residual DOF, but that would be very breaking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nalimilan nalimilan nalimilan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

About dof_residual
2 participants
@andreasnoack @nalimilan